nit: check spelling (here and in commit message)

(also spelling of _ZulipAppState.initState in the commit message)

Did you mean to move this implementation comment here and delete the dartdoc? The implementation comment doesn't make sense in this context—saying GlobalStoreWidget is "separate from" GlobalStoreWidget.

We normally do this as testBinding.getGlobalStoreUniquely, right?

Also, for these tests that aren't specifically about GlobalStoreWidget, it would be simpler to use TestZulipApp instead, I think.

Maybe we can use TestZulipApp for this test?

Used TestZulipApp in tests where possible, but kept this unchanged because I was getting the same extraneous dep changes mentioned in the above TODO.

The comment is wrong about Android; we do offer notifications on Android.

A lot of the code added in this commit is implemented just for iOS, but named/documented as though it's cross-platform, because it doesn't say it's just for iOS.

I don't know if we plan to align the implementation with the names/docs or vice versa. The answer might be in the later commits (I haven't read them yet), but it would be helpful to comment on this in the commit message, I think.

nit: "its"

Hmm, awkward to have this teardown line so far away from its corresponding setup line.

How about instead passing variant: const TargetPlatformVariant({TargetPlatform.iOS})) to testWidgets?

Maybe we should have an iOS counterpart to

• android: When opening a notification, "back" should not leave the account's context #1210

for this? Or make that a both-platforms issue? That issue says:

(The iOS counterpart is covered by #1147, for navigating at all when a notification is tapped.)

but it seems reasonable to postpone this part of it; we'd just want to keep track of it.

After this PR is merged the potential fix for that issue would work on both platforms, because this PR consolidates the notification routing implementation on both iOS and Android.

nit: "the app encounters a notification tap, while the app is running."

(same comment about maybe using variant: const TargetPlatformVariant({TargetPlatform.iOS}))

This needs an update for the Pigeon 25 upgrade e2aac35.

nit: "its"

Commit-message nit:

pigeon [nfc]: Rename pigeon file to `notification` -> `android_notifications`

I think the "to" should be deleted? Or moved to replace the "->"?

pigeon [nfc]: Rename pigeon file `notifications.dart` to `android_notifications.dart`

commit-message nit: limit summary line length to 76 (this is 85).

Can be a bit more concise I think:

  /// Returns `launchOptions.remoteNotification`,
  /// which is the raw APNs data dictionary
  /// if the app launch was opened by a notification tap,
  /// else null. See Apple doc:
  ///   https://developer.apple.com/documentation/uikit/uiapplication/launchoptionskey/remotenotification

And this is only used on iOS at this commit, right; the "On iOS" can be added in the later commit where it also starts being used on Android.

nit: getter before private field: https://github.com/flutter/flutter/blob/master/docs/contributing/Style-guide-for-Flutter-repo.md#order-other-class-members-in-a-way-that-makes-sense

Also, can use fewer lines:

  @override
  FakeNotificationPigeonApi get notificationPigeonApi =>
    (_notificationPigeonApi ??= FakeNotificationPigeonApi());

Instead of the one class NotificationPayloadForOpen, could we have two separate classes NotificationDataFromLaunch and NotificationTapEvent? Perhaps with a Payload typedef helper:

typedef Payload = Map<Object?, Object?>;

class NotificationDataFromLaunch {
  const NotificationDataFromLaunch({required this.payload});

  /// The raw payload that is attached to the notification,
  /// holding the information required to carry out the navigation.
  final Payload payload;
}

class NotificationTapEvent {
  const NotificationTapEvent({required this.payload});

  /// The raw payload that is attached to the notification,
  /// holding the information required to carry out the navigation.
  final Payload payload;
}

I think the current naming makes the event-channel code harder to read than it needs to be. When reading the Pigeon example code, I see "event" used pretty consistently in the names of things. Here, we have both "data" (onNotificationTapEvent's param) and "payload", and "payload" is used ambiguously for a payload (NotificationPayloadForOpen.payload) and something that contains a payload (NotificationPayloadForOpen itself).

That would mean, for this method:

  func onNotificationTapEvent(event: NotificationTapEvent) {
    if let eventSink = eventSink {
      eventSink.success(event)
    }
  }

Would NotificationEventChannelApi be a better name for this, to make it clearer what kind of thing it is?

Is _notifInteractionHost the best name for this? How about _hostApi? We might add more to NotificationHostApi that's not about interacting with notifications (such as a method to query the current notification permissions). Also, NotificationHostApi isn't the only code that's about interacting with notifications; notif_pigeon.notificationTapEvents is too.

Another reason the NotificationPayloadForOpen rename would be helpful: currently, it's pretty unclear that this private method is only about notifications that come while the app is open. It'll be helpful for debugging if it's easier to see what code is about the launch notification vs. not.

This add-self-account line looks boring; can we put it in prepare?

similar to #1379 (comment)

This comment doesn't seem accurate. This condition will indeed occur if the data for the initial route specified an account which didn't match any existing accounts… but it will also occur if the data for the initial route didn't specify any account at all.

In particular I believe this condition will occur every time the app is launched other than by opening a notification.

nit: stray diff hunk

notif [nfc]: Introduce NotificationNavigationServer

And move the notification navigation data parsing utilities to
the new class.

This change doesn't look like it's NFC (see definition). If the URL doesn't parse, the old code would throw (because it called parseAndroidNotificationUrl directly); this version will instead pop up an error dialog.

The dialog seems like a fine change. That change should go in a different commit, though (either before or after). The great value of the NFC concept is that knowing a given commit is supposed to be NFC can help make it a lot simpler to read: all the logic in the red - parts of the diff will match up, not necessarily textually but semantically, with logic in the green + parts of the same diff. The reader can therefore mentally cross off all the bits that match up, aiming to get to zero.

I like this diff hunk — it expresses pretty crisply the fact that before this change, we didn't respond to an initial route on iOS (only on Android), and after this change, we do.

In Future terminology, "complete" includes both success and failure. See e.g. [Future.whenComplete].

(also includes nit: omit "returns" for describing getter; a getter impersonates a field, so it just "is" its value; and "wasn't" doesn't sound right to me tense-wise)

For the name, how about just initialized? It's common for things of type Future to have names that describe the future's result, letting the type speak for the fact that it's a future.

(Re NotificationNavigationData, we'll probably want that to end up covering the more general "open" concept too: in particular we'll need the message ID from the notification, and that will naturally become another field on that class. That more general concept is reflected in the name of the existing NotificationOpenPayload, and in the dartdoc this version has which I supplied at #1379 (comment) above.)

Can this be static? Seems similar to tryParseAndroidNotificationUrl just below it.

Looks like this _navigateForNotification method should get an assert that the platform is iOS, to clarify why this line makes sense.

Can this method be static? Doesn't look like it's using any instance data.

Hmm yeah, good thought reordering these while you're here editing them.

It'd be good if we can avoid going and adding another detail like this that tests have to know about.

How about having NotificationService.debugReset call the new service's debugReset? Seems appropriate for the main service to take responsibility for resetting the new one, given that the main service is also the one place responsible for starting up the new service.

Is this line needed? I'd think this data wouldn't be involved in any of these tests.

Yes, otherwise we hit this assert here:

Because ZulipBinding.firebaseMessaging.getToken ends up being called in NotificationService.start.

Since the code this is testing is getting moved between files, the tests should move to remain in a corresponding file.

Hmm, does this function have tests? I don't see them. (I do see a variety of test cases for the logic downstream of here, which is good.)

This logic is fairly densely packed with fiddly details, like parsing logic tends to be, so it's logic that's valuable to have good tests for. And, again typically for parsing logic, it has a pretty crisp interface for both input and output, which makes it nicely amenable to writing plenty of unit tests. So let's go ahead and do that.

For a point of comparison, see the existing tests of NotificationOpenPayload in display_test.dart.

I don't understand what a ";" means here.

Is this comment accurate? I don't see anything about PlatformDispatcher.defaultRouteName here.

It looks like this test case is copied from an existing one in display_test.dart. But that one says:

      // Set up a value for `PlatformDispatcher.defaultRouteName` to return,
      // for determining the initial route.
      final account = eg.selfAccount;
      final message = eg.streamMessage();
      final data = messageFcmMessage(message, account: account);
      final intentDataUrl = NotificationNavigationData(
        /* … */).buildAndroidNotificationUrl();
      addTearDown(tester.binding.platformDispatcher.clearDefaultRouteNameTestValue);
      tester.binding.platformDispatcher.defaultRouteNameTestValue = intentDataUrl.toString();

so PlatformDispatcher.defaultRouteName really is what it's about.

Speaking of this test, it looks like it (and several of its neighbors) should also move to navigate_test.dart / open_test.dart as part of the code-moving commit — the code they're testing is part of what's getting moved.

(In particular the way these tests exercise the code they're testing is via ZulipApp — and the relevant call sites in ZulipApp switch from NotificationDisplayManager methods to NotificationNavigationService methods.)

As a bonus, those moves may allow you to outright move some of the helpers from this test file, rather than copy them as the current version does.

This navigateForNotification method seems like it should move too — it's logically closely related to the other methods like routeForNotification which are moving.

It's also quite similar to the _navigateForNotification method which the last commit introduces on the other class. It might be fine to have some code duplication between these two methods — but if we do that, it's definitely better if they're right next to each other, so that the parallelism is more visible.